-
Notifications
You must be signed in to change notification settings - Fork 4
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
locksmithcl: reset endpoints
when we set a new one
#13
Conversation
This behavior was working fine previously but no more with the etcd/v2 upgrade. In the case we have an endpoint like `https://127.0.0.1:2379`, this one will be _added_ to the list of `defaultEndpoints` which lead to this endpoints: ```go []string{ "http://127.0.0.1:2379", "http://127.0.0.1:4001", "https://127.0.0.1:2379", } ``` It's not an issue when we have a retry mechanism - but in this case if `etcd` use HTTPS and we try an `HTTP` endpoint, we will get an io.EOF error. Signed-off-by: Mathieu Tortuyaux <mathieu@kinvolk.io>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What about the code here: https://github.com/kinvolk/locksmith/blob/9287272dff87cb8c35eafeff9dcc91190d0b5ba6/locksmithctl/locksmithctl.go#L138-L140? Isn't it sufficient?
@invidian - codebase is a bit confusing. In the snippet you shared, we set globalFlags.Endpoints = []string{
"http://127.0.0.1:2379",
"http://127.0.0.1:4001",
} Later, in the code, if we run in This method will pass through all the If we have [Unit]\nAfter=etcd-member.service\nRequires=etcd-member.service\n[Service]\nEnvironment=LOCKSMITHD_ETCD_CERTFILE=/etc/ssl/certs/locksmith-cert.pem\nEnvironment=LOCKSMITHD_ETCD_KEYFILE=/etc/ssl/certs/locksmith-key.pem\nEnvironment=LOCKSMITHD_ETCD_CAFILE=/etc/ssl/certs/ca-etcd-cert.pem\nEnvironment=LOCKSMITHD_ENDPOINT=https://localhost:2379\nEnvironment=LOCKSMITHD_REBOOT_WINDOW_START=00:00\nEnvironment=LOCKSMITHD_REBOOT_WINDOW_LENGTH=23h59m" and since the
So the |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, considering that main()
in locksmithctl/locksmithctl.go
should most likely be splitted into 2 different functions.
This behavior was working fine previously but no more with the etcd/v2
upgrade.
In the case we have an endpoint like
https://127.0.0.1:2379
, this onewill be added to the list of
defaultEndpoints
which lead to thisendpoints:
It's not an issue when we have a retry mechanism - but in this case if
etcd
use HTTPS and we try anHTTP
endpoint, we will get an io.EOFerror.
Signed-off-by: Mathieu Tortuyaux mathieu@kinvolk.io
Otherwise, we could just do this:
but, as a user, I would expect that if I run
locksmithctl -endpoint https://127.0.0.1:2379
I would run locksmithcl only against this endpoint, not a bundle ofdefautEndpoints + endpoint
.This issue has been caught by the CI with the
coreos.locksmith.tls
- I will rerun the kola tests before merging this one.